Hello @quinor! Thanks for updating this PR.

• In the file pytorch_lightning/profiler/__init__.py:

Line 75:121: E501 line too long (132 > 120 characters)
Line 76:121: E501 line too long (133 > 120 characters)
Line 77:121: E501 line too long (132 > 120 characters)
Line 97:121: E501 line too long (132 > 120 characters)

Comment last updated at 2020-12-02 23:27:31 UTC

@quinor what do need help with? @jeremyjordan may you have look?

Tests, docs and thread safety + (of course) a review. Please see the list at the end of the top message.

@Borda would you mind looking into the thread safety?

This pull request is now in conflict... :(

Borda would you mind looking into the thread-safety?

what do you mean by thread-safety, in python unless it is spec by no-GIL locker it is always safe...

Borda would you mind looking into the thread-safety?

what do you mean by thread-safety, in python unless it is spec by no-GIL locker it is always safe...

Quoted from my initial PR message:

"thread-safety (the profiler may fail if it's ran in multiple threads, ie. when there's more than one dataloader worker the profiler can be entered with the same action_name multiple times at once and that doesn't end well)"

Basically if someone does:

[thread1] profiler.start("name")
[thread2] profiler.start("name")
[thread1] profiler.stop("name")
[thread2] profiler.stop("name")

my profiler does break. There are 3 options I see right now:

1. fix this profiler so that it's OK
2. fix the way profilers are called in PL so that it doesn't happen
3. (I think might be the best) add some kind of UID to the profiler.{start, stop} calls so that each start-stop pair is unique. Alternatively make start return an object "stop" is called upon, that would require a bit of code rework though.

Hey @quinor, do you still need help with this? Mind taking a look?

@edenlightning please see my last message. I'm not deep enough into lightning to know how to fix the issues I had pinpointed and I don't think it can be merged without fixing them, so help with that would really be appreciated. I thought @Borda was going to look into that.

@Borda keep this PR?

Just came across this linked in the Slack - this would be very cool functionality if it could end up merged.

Just came across this linked in the Slack - this would be very cool functionality if it could end up merged.

feel free to work on it, I'm not deep enough in lightning to fix the issues I pinpointed :/

I'm not either unfortunately, but the issue with the profiler and num_workers > 0 seems like it may have deeper roots: pytorch/pytorch#6313

I 'll have a look just when we fix master so we can easily debug the optional issue here...

Codecov Report

Merging #1693 (ddf8fb8) into master (7b7d4bb) will decrease coverage by 0%.
The diff coverage is 96%.

@@          Coverage Diff           @@
##           master   #1693   +/-   ##
======================================
- Coverage      93%     93%   -0%     
======================================
  Files         124     124           
  Lines        9303    9355   +52     
======================================
+ Hits         8616    8662   +46     
- Misses        687     693    +6     

@quinor how is it going, do you this we gen this done by end of this week to pass to 1.1? 🐰

@Borda I'll try to get the code ready on wednesday + hopefully some docs. I hope we can then close it by the end of the week.

I see some tests failing for pytorch 1.3 (since old pytorch's autograd.profile does not support profile_memory) - what should we do with that? It's supported in pytorch since PR pytorch/pytorch#37775, that is pytorch 1.6 I think...

@Borda it's ready from my side, some issues (tests failing due to old pytorch) still pending that I couldn't resolve by myself. Do you still want to push it into 1.1?

Hey @quinor,

I have a similar PR there: #5560. Let's merge them together.
Please, join Slack so we can coordinate there :)

Best,
T.C

closing, #5560 implements this profiler.